Skip to content

fix(generic): escape SQL identifiers and literals in metadata/DDL paths (#1914) - #2199

Merged
openai0229 merged 5 commits into
OtterMind:mainfrom
HandSonic:fix/sqli2-generic
Jul 29, 2026
Merged

fix(generic): escape SQL identifiers and literals in metadata/DDL paths (#1914)#2199
openai0229 merged 5 commits into
OtterMind:mainfrom
HandSonic:fix/sqli2-generic

Conversation

@HandSonic

@HandSonic HandSonic commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Related issue

Closes #1914

Summary

  • Harden Generic database and table DDL template substitution against second-order SQL injection from database metadata.
  • Inspect each configured SQL template before substitution: values in single-quoted placeholder positions are escaped as string-literal content, while values in bare placeholder positions must be strict identifier tokens because the Generic adapter cannot assume one dialect quote character.
  • Register GenericIdentifierProcessor for SPI consumers, keeping ordinary quoteIdentifier conditional while implementing unconditional, delimiter-safe quoteIdentifierAlways with exact quote/remove round-trip behavior.
  • Preserve the original database, schema, and table names for the JDBC metadata fallback; only the configured SQL-template path receives escaped template values.
  • Add focused tests for DuckDB literal templates, TDengine bare-identifier templates, malicious inputs, conditional quoting, boundary delimiters, blank values, and exact round trips.

Affected surfaces

  • Frontend / Web
  • Backend / API / Storage
  • Database plugin / Driver
  • JCEF / Desktop packaging
  • CI / Build / Release
  • Documentation only

Verification

  • Commands and results:
    • mvn -B -f chat2db-community-server/pom.xml -pl chat2db-community-plugins/chat2db-community-generic -am clean test -Dmaven.test.skip=false -DskipTests=false -Dmaven.test.failure.ignore=false
    • Result: 13-module reactor BUILD SUCCESS; tools 26/26, SPI 90/90, MySQL 653/653, PostgreSQL 49/49, DuckDB 2/2, TDengine 1/1, Generic 15/15; zero failures, errors, or skipped tests in these modules. Elasticsearch and domain-api compiled successfully and contain no tests in this reactor.
    • git diff --check origin/main...HEAD
    • Result: passed with no whitespace errors.
  • Manual verification: N/A; the affected SQL generation and identifier contracts are covered by executable unit tests, and no live Generic database is required for the template-substitution logic.
  • UI evidence: N/A

Risk and compatibility

  • Public API or stored data: No stored-data migration. The Generic plugin now exposes its identifier processor through the existing SPI contract; no new public endpoint is added.
  • Database or driver compatibility: DuckDB single-quoted metadata lookups retain arbitrary object-name support through literal escaping. TDengine bare template positions now reject names outside [A-Za-z0-9_$]+ because Generic cannot safely infer a dialect delimiter there.
  • Network, privacy, or security: Reduces second-order SQL-injection risk in Generic metadata and database-switch templates. No network or privacy behavior changes.
  • Community / Local / Pro boundary: Community Generic plugin only; Local or Pro products consume the change only when built from this Community plugin.
  • Backward compatibility: Normal identifiers and existing configured templates remain supported. Unsafe or dialect-ambiguous bare identifiers now fail fast instead of being interpolated into SQL.

Reviewer map

  • Start here: chat2db-community-server/chat2db-community-plugins/chat2db-community-generic/src/main/java/ai/chat2db/plugin/generic/GenericSqlGuards.java, then GenericMetaData.java, GenericDBManager.java, and identifier/GenericIdentifierProcessor.java.
  • Failure condition: Any configured template receives an unescaped single-quoted value, accepts an unsafe bare identifier, mutates the original JDBC fallback arguments, or violates removeIdentifierQuote(quoteIdentifierAlways(raw)).equals(raw).
  • Rollback or disable path: Revert this PR. There is no feature flag or persistent-data migration.

Contributor declaration

  • I linked the Issue that defines this change.
  • I tested the affected behavior and reported the actual results above.
  • I did not include credentials, private data, or generated build output.
  • I disclosed substantial AI assistance below, or this PR contains no substantial AI-generated code.

AI assistance: Maintainer used AI-assisted code review and test development; all resulting changes were source-reviewed and verified with the clean Maven reactor reported above.

HandSonic and others added 5 commits July 29, 2026 20:17
…maintainer review (OtterMind#1914)

- new GenericIdentifierProcessor (SPI ISQLIdentifierProcessor): quoteIdentifier
  with double-quote default + dialect-parameterized char variant, escapeString
  with single-quote doubling
- GenericMetaData overrides getSQLIdentifierProcessor()
- template-aware validation moved to GenericSqlGuards (sanitizeTemplateValue,
  requireSafeIdentifier AS-IS), escaping delegated to the processor
- GenericSqlEscapes removed; tests migrated (12 green)

@openai0229 openai0229 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintainer review complete: Generic template values are context-aware, JDBC fallback arguments remain raw, the identifier processor satisfies the shared always-quote round-trip contract, and the 13-module clean reactor passed.

@openai0229
openai0229 merged commit 244590e into OtterMind:main Jul 29, 2026
19 checks passed
@openai0229 openai0229 moved this from In Review to Done in Chat2DB Community Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Security: 34 SQL Injection Vulnerabilities in Database Plugins

2 participants